feat(bug-detectors): replace RCE substring check with global canary#873
feat(bug-detectors): replace RCE substring check with global canary#873
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Remote Code Execution bug detector to avoid false positives from substring checks by introducing a global canary on globalThis and ensuring it’s also visible inside Jest’s vm.Context sandbox.
Changes:
- Replace source-text substring detection for
eval/Functionwith a global canary-based detection mechanism. - Update before-hooks to provide only fuzzing guidance (
guideTowardsContainment) instead of reporting findings directly. - Expand and rename fuzz targets/tests to cover canary-triggering cases and “safe” (non-triggering) cases, including string-literal and string-coercible bodies.
Reviewed changes
Copilot reviewed 61 out of 63 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/bug-detectors/remote-code-execution/tests.fuzz.js | Renames/reorganizes fuzz test cases to distinguish canary-triggering vs safe scenarios. |
| tests/bug-detectors/remote-code-execution/fuzz.js | Updates fuzz entrypoints to execute code that should (or should not) trigger the canary for eval and Function. |
| tests/bug-detectors/remote-code-execution.test.js | Aligns CLI + Jest integration tests with the new entrypoints and expected outcomes. |
| packages/bug-detectors/internal/remote-code-execution.ts | Implements the global canary, propagates it to Jest vmContext, and changes hooks to guidance-only. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The current design is better than looking for "jaz_zer" substring in the source text for const propertyName = "jaz_zer";
void globalThis[propertyName];
console.log("can be called just fine");I think that the current “if anything reads globalThis.jaz_zer, report RCE.” check is too borad. How about we create a cannary call, e.g., |
|
I think we should make it configurable and allow both (as in either or) defaulting to the current implementation that marks access as RCE. And the reasoning is that it is:
var fn = eval(my_awesome_sanitizer("..."));
...
// much later, after lots of fuzzing
fn(); // actual callIn case it's a false-positive, the users can be informed that it's possible to switch to a more strict RCE detection that requires an actual canary execution. |
Mock the parent directory walk for the missing package.json case so the corpus test stays independent of tmp ancestors on the local machine. Type the readdirSync mock explicitly so TypeScript keeps compiling the test.
Replace the brittle substring detector with deterministic canaries for eval and Function. Split findings into canary access and invocation, and emit copy-paste suppression rules for noisy heuristic reads.
The old detector checked whether the target string appeared in eval/Function source text. This false-positived on safely quoted occurrences (e.g. Handlebars' lookupProperty(d, "jaz_zer")).
Install a canary function on globalThis that fires only when attacker-controlled code actually executes. The before-hooks now provide fuzzing guidance only (guideTowardsContainment). The canary is propagated to the Jest vm.Context sandbox, following the same pattern as the prototype-pollution detector.